[RelayMiner]: add `proxy.Ping(...)` capability to test connectivity between relay servers and backend URLs #1037

eddyzags · 2025-01-21T17:12:37Z

Summary

This PR adds the capability to test the connectivity between the Relay Servers and the Backend URLs in two ways.

Safeguard at Startup:
For every suppliers.[].service_config.backend_url referenced as input inside the Relay Miner Configuration file, the Relay Proxy will verify wether the network connection between the targeted backend_url and the relayerminer process is functioning properly. If one or more connections aren't possible, the relay miner won't be able to start.
Configurable Ping HTTP server:
The Relay Miner process will listen for incoming request to synchronously test the connectivity of every referenced suppliers.[].service_config.backend_url. If one or more backend URLs aren't reachable, the incoming request will fail.

Based on the serverConfig.ServerType (Example: HTTP), each Server Type will implement their own logic to implement to test the connectivity.

Issue

[RelayMiner] Add an ability to test RelayMiner configuration without application #447

Type of change

Select one or more:

Testing

Documentation changes (only if making doc changes)

make docusaurus_start; only needed if you make doc changes

Local Testing (only if making code changes)

Unit Tests: make go_develop_and_test
LocalNet E2E Tests: make test_e2e
See quickstart guide for instructions

PR Testing (only if making code changes)

DevNet E2E Tests: Add the devnet-test-e2e label to the PR.
- THIS IS VERY EXPENSIVE, so only do it after all the reviews are complete.
- Optionally run make trigger_ci if you want to re-trigger tests without any code changes
- If tests fail, try re-running failed tests only using the GitHub UI as shown here

Sanity Checklist

I have tested my changes using the available tooling
I have commented my code
I have performed a self-review of my own code; both comments & source code
I create and reference any new tickets, if applicable
I have left TODOs throughout the codebase, if applicable

Summary by CodeRabbit

New Features
- Introduced a new configuration section for the ping functionality, allowing users to test backend connectivity within the relay miner's setup.
- Added methods to handle ping requests, enhancing health check capabilities for relay servers.
Bug Fixes
- Improved error handling during the server startup process if any relay server is unreachable.
Tests
- Added tests for the new ping functionality to ensure operational integrity and reliability of the relay miner.

bryanchriswhite

Thanks for picking this back up @eddyzags! 🙌

I have to stop here for today but this is looking great so far! 🚀
The biggest thing I haven't reviewed yet is the test (but I already saw the addition of go-mockdns, and I skimmed the test names 😉) and am looking forward to it.

bryanchriswhite · 2025-01-24T10:45:57Z

Tiltfile

Was this change intentionally persisted, and if so, how is it related to this feature?

I think this change should be reverted. My assumption is that this is the result of an older commit which was never reconciled completely with main:

The yaml files referenced don't exist.

The flags seem to be specifying the same/similar config as what's been removed from the relayminer configs that do exist. 🤔

Sorry, I wasn't clear in my previous comments.

Was this change intentionally persisted, and if so, how is it related to this feature?

Yes, this change was intentionally made to ensure the Ping safeguard at startup succeeds for the Relayminer with the localnet default configuration, and/or any custom localnet configuration in that regard (link to localnet default configuration in the main branch). In the default localnet configuration, the Ollama Kubernetes deployment is not applied (ollama.enabled=false). However, the relayminer configuration still referenced Ollama suppliers in its configuration files, even though the container wasn’t deployed (link to relayminer-1 configuration for localnet). With the newly introduced mechanism of the Ping safeguard at startup, this will cause the relayminer to fail continuously because the Ollama container isn't deployed.

To solve this issue, I found a way to dynamically define the relayminer's configuration based on the localnet configuration by modifying the poktrolld/Tiltfile. Hence, those modifications.

For poktrolld users that are deploying a Relayminer without relying on the localnet, they will have to make sure that their config.suppliers[*].service_config.backend_url are up and running and reachable before deploying a Relayminer.

The yaml files referenced don't exist.

I disagree, they exists:

values-common.yaml is defined here

values-relayminer-common.yaml is defined here

values-relayminer- + str(actor_number) + ".yaml" is defined here, here and here

The flags seem to be specifying the same/similar config as what's been removed from the relayminer configs that do exist.

I cannot find that. Can you link me to the precise line in my fork that makes you think that please? 🙏🏾

@eddyzags thanks for the detailed response here! 🙌

The yaml files referenced don't exist.

I was referring to .yaml files referenced in this commit, but I also see that they're not referenced any more. I just didn't understand the rationale behind moving the config into the Tiltfile.

(@okdas @red-0ne thoughts?)

The flags seem to be specifying the same/similar config as what's been removed from the relayminer configs that do exist.

I was just pointing out that the config fields which you've removed from the relayminer configs correspond to the flags you've added in the Tiltfile. The point being, to question why should we prefer to provide the config via flags over the yaml file, which you answered.

Got it, Bryan; I am glad it was clear. Waiting for @okdas and @red-0ne feedback. I am open to suggestions

@eddyzags This LGTM but #PUC in the code with your explanation related to Ping safeguard.

You already have it written down anyhow :)

Tiltfile

localnet/kubernetes/values-relayminer-1.yaml

docusaurus/docs/operate/configs/relayminer_config.md

pkg/relayer/proxy/synchronous.go

pkg/relayer/relayminer_test.go

bryanchriswhite · 2025-01-24T13:31:17Z

testutil/testproxy/relayerproxy.go

-				server.Handler = http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
-					sendJSONRPCResponse(test.t, w)
-				})
+				listener, err := net.Listen("tcp", supplierConfig.ServiceConfig.BackendUrl.Host)


Why separate the listener from the server?

By using a custom listener, and thereby decoupling the listener from the serve action, we ensure that the HTTP server is fully prepared to listen on a specific port in the test's main Go routine. This guarantees that the HTTP server(s) is ready before proceeding to the actual test cases.

Previously, listening and serving were handled within the Go routine using http.ListenAndServe function. This approach sometimes led to the HTTP server not being ready when the test cases began execution, resulting in test failures and flaky behavior.

Amazing! 👍 #PUC with that explanation, perhaps condensed, if possible.

eddyzags · 2025-01-27T15:04:36Z

Thanks for reviewing @bryanchriswhite ! Waiting for the rest of the review 🚀

bryanchriswhite

Amazing work @eddyzags! 🙌 🚀

I had a few more suggestions and I would like to get some feedback from @okdas and @red-0ne, but otherwise I think this is just about ready to 🚢. 😎

bryanchriswhite · 2025-01-29T09:33:50Z

Tiltfile

@eddyzags thanks for the detailed response here! 🙌

The yaml files referenced don't exist.

I was referring to .yaml files referenced in this commit, but I also see that they're not referenced any more. I just didn't understand the rationale behind moving the config into the Tiltfile.

(@okdas @red-0ne thoughts?)

The flags seem to be specifying the same/similar config as what's been removed from the relayminer configs that do exist.

I was just pointing out that the config fields which you've removed from the relayminer configs correspond to the flags you've added in the Tiltfile. The point being, to question why should we prefer to provide the config via flags over the yaml file, which you answered.

localnet/kubernetes/values-relayminer-1.yaml

pkg/relayer/proxy/proxy_test.go

bryanchriswhite · 2025-01-29T10:43:51Z

pkg/relayer/proxy/proxy_test.go

+	relayProxyBehavior := append(t.relayerProxyBehavior, []func(*testproxy.TestBehavior){
+		testproxy.WithDefaultSupplier(t.supplierOperatorPingAllKeyName, supplierEndpoints),
+		testproxy.WithServicesConfigMap(servicesConfigMap),
+	}...)


#PUC options provided via multiple calls to testproxy.WithDefaultSupplier(...) and testproxy.WithServicesConfigMap(...) are not mutually exclusive; the former accumulates service endpoints into a testutil global variable, and the latter starts an http server for each service in the given services config map.

I'm sorry, but I'm not sure I understand this. I aimed to define two relay servers with their own suppliers and services managed by the same relay proxy. Then call PingAll to see if the mechanism is working across multiple relay servers.

I've refactored this part by using testproxy.WithDefaultSupplier(...) & testproxy.WithServicesConfigMap(...) one time only. eddyzags@598f85e

bryanchriswhite · 2025-01-29T11:02:38Z

pkg/relayer/relayminer.go

+	return nil
+}
+
+func (rel *relayMiner) newPinghandlerFn(ctx context.Context, ln net.Listener) http.HandlerFunc {


It seems like the ln variable is unused.

Suggested change

func (rel *relayMiner) newPinghandlerFn(ctx context.Context, ln net.Listener) http.HandlerFunc {

func (rel *relayMiner) newPinghandlerFn(ctx context.Context) http.HandlerFunc {

bryanchriswhite · 2025-01-29T11:02:45Z

pkg/relayer/relayminer.go

+	// ping requests. A single ping request on the relay server broadcasts a
+	// ping to all backing services/data nodes.
+	go func() {
+		if err := http.Serve(ln, rel.newPinghandlerFn(ctx, ln)); err != nil && !errors.Is(http.ErrServerClosed, err) {


Suggested change

if err := http.Serve(ln, rel.newPinghandlerFn(ctx, ln)); err != nil && !errors.Is(http.ErrServerClosed, err) {

if err := http.Serve(ln, rel.newPinghandlerFn(ctx)); err != nil && !errors.Is(http.ErrServerClosed, err) {

pkg/relayer/relayminer.go

bryanchriswhite · 2025-01-29T11:11:51Z

pkg/relayer/relayminer_test.go

@@ -57,3 +60,69 @@ func TestRelayMiner_StartAndStop(t *testing.T) {
 	err = relayminer.Stop(ctx)
 	require.NoError(t, err)
 }
+
+func TestRelayMiner_Ping(t *testing.T) {


Would you mind adding one more test for the error cases where the relayer proxy mock's #PingAll() returns the temporary and non-temporary *url.Errors such that we can assert (and cover regression of) the resulting error returned from the HTTP GET on the ping endpoint?

Agree. Added here eddyzags@43838e7 (Refactor to test suite here: eddyzags@4c5bc4b)

bryanchriswhite · 2025-01-29T11:13:27Z

localnet/poktrolld/config/relayminer_config.yaml

(cc @okdas)

…ty between relay servers and backend URLs (#1) * relayer: add RelayServers() method to RelayProxy interface; Add Ping(), ServiceIDs(), Forward() method to RelayServer interface; add RelayServers slice with helper method byServiceID * relayer: add forward config entry * relayer: implement ServiceIDs, Forward, and Ping method for synchrounous RPC server * relayer: add RelayServers implementation for RelayProxy * relayer: add Ping and Forward options * relayer: integrate ping option * relayer: add ServePing and ServeForward method to RelayMiner * test proxy.Ping() in test + remove forward feature * add serve ping test * add doc

…s based on localnet config

…tfile

… avoid flaky tests

…ping test suite

…e error checking

Olshansk

@eddyzags Wasn't my intention to have this hanging for so long, but I'm glad @bryanchriswhite and you had a good back & forth to get it here.

In terms of next step:

Please see @bryanchriswhite's comments
See my minor NITs
Merge with the latest main
@okdas will review the one TiltFile / k8s related comment
Can you upload a video to the github PR description showing this

Especially as PGAT is getting kicked off (and we have some large beta users), I think everyone will love this!

Olshansk · 2025-02-13T19:05:09Z

Makefile

+### Localnet Helpers ###
+########################
+
+.PHONY: localnet_relayminer1_ping


Can you move these into a ping.mk and add an import at the obbtom of this makefile?

Olshansk · 2025-02-13T19:07:31Z

localnet/kubernetes/values-relayminer-1.yaml

-        backend_url: http://rest:10000/
-        publicly_exposed_endpoints:
-          - relayminer1
+  suppliers: [] # suppliers list is dynamically defined in poktroll/Tiltfile.


I just want a 👍 from @okdas in case this has downstream effects on our E2E testing in ephemeral DevNets.

I don't see this breaking e2e on devnets.

Olshansk · 2025-02-13T19:07:59Z

docusaurus/docs/operate/configs/relayminer_config.md

+Configures a `ping` server to test the connectivity of all backend URLs. If
+all the backend URLs are reachable, the endpoint returns a 204 HTTP
+Code. If one or more backend URLs aren't reachable, the service
+returns an appropriate HTTP error.


Suggested change

Configures a `ping` server to test the connectivity of all backend URLs. If

all the backend URLs are reachable, the endpoint returns a 204 HTTP

Code. If one or more backend URLs aren't reachable, the service

returns an appropriate HTTP error.

// ConfigurePingHandler sets up a health check server that:

// - Tests connectivity to all configured backend URLs

// - Returns HTTP 204 if all backends are reachable

// - Returns appropriate HTTP error if any backend is unreachable

Olshansk · 2025-02-13T19:08:50Z

Tiltfile

@eddyzags This LGTM but #PUC in the code with your explanation related to Ping safeguard.

You already have it written down anyhow :)

Olshansk · 2025-02-13T19:09:44Z

testutil/testrelayer/proxy.go

+// NewMockOneTimeRelayerProxyWithPing creates a new mock RelayerProxy. This mock
+// RelayerProxy will expect a call to ServedRelays with the given context, and
+// when that call is made, returnedRelaysObs is returned. It also expects a call
+// to Start, Ping, and Stop with the given context.


Suggested change

// NewMockOneTimeRelayerProxyWithPing creates a new mock RelayerProxy. This mock

// RelayerProxy will expect a call to ServedRelays with the given context, and

// when that call is made, returnedRelaysObs is returned. It also expects a call

// to Start, Ping, and Stop with the given context.

// NewMockOneTimeRelayerProxyWithPing creates a new mock RelayerProxy that:

// - Expects a call to ServedRelays with the given context

// - Returns returnedRelaysObs when ServedRelays is called

// - Expects one call each to Start, Ping, and Stop with the given context

Olshansk · 2025-02-13T19:11:39Z

pkg/relayer/relayminer.go

+	}()
+
+	go func() {
+		<-ctx.Done()


#PUC when we expect this context to close.

I'm guessing (assumption, gut intuition) is when the process shuts down, but making it explicit would be nice.

Olshansk · 2025-02-13T19:11:51Z

pkg/relayer/relayminer.go

+	// Start a long-lived goroutine that starts an HTTP server responding to
+	// ping requests. A single ping request on the relay server broadcasts a
+	// ping to all backing services/data nodes.


Suggested change

// Start a long-lived goroutine that starts an HTTP server responding to

// ping requests. A single ping request on the relay server broadcasts a

// ping to all backing services/data nodes.

// StartPingServer launches a goroutine that:

// - Creates a long-running HTTP server

// - Handles ping requests by broadcasting health checks to all backing services

// - Tests connectivity to all configured data nodes

Olshansk · 2025-02-13T19:13:18Z

pkg/relayer/config/types.go

+
+// RelayMinerPingConfig is the structure resulting from parsing the ping
+// server configuration.
+type RelayMinerPingConfig struct {


Suggested change

type RelayMinerPingConfig struct {

// TODO_TECHDEBT(@red-0ne): Remove this structure altogether. See the discussion here for ref:

// https://github.com/pokt-network/poktroll/pull/1037/files#r1928599958

type RelayMinerPingConfig struct {

Olshansk · 2025-02-13T19:13:43Z

pkg/relayer/config/relayminer_configs_reader.go

+	relayMinerConfig.Ping = &RelayMinerPingConfig{
+		Enabled: yamlRelayMinerConfig.Ping.Enabled,
+		Addr:    yamlRelayMinerConfig.Ping.Addr,
+	}


Let's make this change.

Olshansk · 2025-02-13T19:13:52Z

pkg/relayer/proxy/proxy.go

@@ -206,3 +214,22 @@ func (rp *relayerProxy) validateConfig() error {

 	return nil
 }
+
+// PingAll tests the connectivity between all the managed relay servers and their respective backend URLs.
+func (rp *relayerProxy) PingAll(ctx context.Context) error {


okdas

Only checked the Tiltfile and kubernetes yaml part - looks good to me. Thank you @eddyzags!

okdas · 2025-02-13T23:30:25Z

localnet/kubernetes/values-relayminer-1.yaml

-        backend_url: http://rest:10000/
-        publicly_exposed_endpoints:
-          - relayminer1
+  suppliers: [] # suppliers list is dynamically defined in poktroll/Tiltfile.


I don't see this breaking e2e on devnets.

eddyzags mentioned this pull request Jan 21, 2025

[RelayMiner]: add proxy.Ping(...) capability to test connectivity between relay servers and backend URLs #744

Closed

14 tasks

Olshansk requested review from Olshansk, bryanchriswhite and red-0ne January 21, 2025 23:05

Olshansk assigned eddyzags Jan 21, 2025

Olshansk added tooling Tooling - CLI, scripts, helpers, off-chain, etc... community A ticket intended to potentially be picked up by a community member nice-to-have Not-important and not-urgent labels Jan 22, 2025

Olshansk added this to the Beta TestNet Iteration milestone Jan 22, 2025

bryanchriswhite requested changes Jan 24, 2025

View reviewed changes

eddyzags force-pushed the main branch from bedf1a1 to 6386078 Compare January 27, 2025 14:24

eddyzags requested a review from bryanchriswhite January 27, 2025 15:04

bryanchriswhite requested changes Jan 29, 2025

View reviewed changes

eddyzags force-pushed the main branch from a100b3f to 43838e7 Compare February 9, 2025 14:45

eddyzags requested a review from bryanchriswhite February 9, 2025 16:02

eddyzags force-pushed the main branch from ddf27ca to b8704a1 Compare February 10, 2025 00:24

eddyzags added 13 commits February 11, 2025 23:55

use dynamic slice in Ping error handling

ecbcb3e

relayer: add godoc to configuration yaml

54b79d1

proxy: add comment explaining application logic

7bd3223

relayer: change Ping to PingAll in relayproxy interface

0234978

proxy: cleanup unused code

affc937

relayer: add godoc explaining ping http serve method

9ddc62a

relayer: remove blank line

8395970

localnet: add ping helpers for relayminers + logic to define supplier…

2df6479

…s based on localnet config

use errors.Join instead of appending errors slice

d3710d8

change c to httpClient in sychronous in relay server

8830010

add relayer miner suppplier not reachable error

86976f1

add newpinghandlerfn function

493884e

eddyzags added 26 commits February 11, 2025 23:55

add endpointURL variable

8130cf3

improve godoc comments for pingconfig

c164306

fix serveping tests + refactor relayminer.ServePing function signature

239f2f1

refactor: dynamically set values for relayminer suppliers list in Til…

385b7e9

…tfile

add pingall test suite

ad0f0a7

add proxy different endpoints ping tests

fe6342a

stabilize flaky helpers in testproxy

d7abf60

add comments and refactor variable names

e35986e

fix typo in Tiltfile

dd6b7e8

fix tyop

ad39a0c

categorized as errors any HTTP status code higher or equal to 400

7f21be9

improve error handling while serving http request for ping server

446d2f2

distinguish 502 from 503 errors in ping handler for error handling

d2ade64

rely on testing temp dir for testing files

53ca20e

improve relayminer configuration documentation

39d1458

improve error message for ping request

07361b9

minor fix

0c11e02

add comments to explain WithServiceConfigMap() function management to…

8988335

… avoid flaky tests

rely on defaultService pkg level constant for ping test suite

2947c2b

remove unused variable in synchrounous Ping method

baa9b61

re-using supplierOperatorKeyName package level constant variable for …

3800411

…ping test suite

add two separate relay servers for all with multiple servers + improv…

acdd9d1

…e error checking

remove unused variables

2f2d9cc

add TestRelayMiner_NOKPing unit test

f4027a5

refactor Relayminer ping test using suite

6b27a83

add optional to ping documentation

cc573c7

eddyzags force-pushed the main branch from b8704a1 to cc573c7 Compare February 11, 2025 22:55

Olshansk requested changes Feb 13, 2025

View reviewed changes

okdas approved these changes Feb 13, 2025

View reviewed changes

Merge branch 'main' into main

bbf05c0

	func (rel *relayMiner) newPinghandlerFn(ctx context.Context, ln net.Listener) http.HandlerFunc {
	func (rel *relayMiner) newPinghandlerFn(ctx context.Context) http.HandlerFunc {

	if err := http.Serve(ln, rel.newPinghandlerFn(ctx, ln)); err != nil && !errors.Is(http.ErrServerClosed, err) {
	if err := http.Serve(ln, rel.newPinghandlerFn(ctx)); err != nil && !errors.Is(http.ErrServerClosed, err) {

-	// Start a long-lived goroutine that starts an HTTP server responding to
-	// ping requests. A single ping request on the relay server broadcasts a
-	// ping to all backing services/data nodes.
+	// StartPingServer launches a goroutine that:
+	// - Creates a long-running HTTP server
+	// - Handles ping requests by broadcasting health checks to all backing services
+	// - Tests connectivity to all configured data nodes

[RelayMiner]: add proxy.Ping(...) capability to test connectivity between relay servers and backend URLs #1037

Are you sure you want to change the base?

[RelayMiner]: add proxy.Ping(...) capability to test connectivity between relay servers and backend URLs #1037

Conversation

eddyzags commented Jan 21, 2025

Summary

Issue

Type of change

Testing

Sanity Checklist

Summary by CodeRabbit

Summary by CodeRabbit

bryanchriswhite left a comment

Choose a reason for hiding this comment

bryanchriswhite Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanchriswhite Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyzags commented Jan 27, 2025

bryanchriswhite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyzags Feb 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okdas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[RelayMiner]: add `proxy.Ping(...)` capability to test connectivity between relay servers and backend URLs #1037

[RelayMiner]: add `proxy.Ping(...)` capability to test connectivity between relay servers and backend URLs #1037

bryanchriswhite Jan 24, 2025 •

edited

Loading

bryanchriswhite Jan 28, 2025 •

edited

Loading

eddyzags Feb 9, 2025 •

edited

Loading